Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(ListView): API Decision #2453

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

docs(ListView): API Decision #2453

wants to merge 16 commits into from

Conversation

saurabhdaware
Copy link
Member

@saurabhdaware saurabhdaware commented Dec 26, 2024

Copy link

changeset-bot bot commented Dec 26, 2024

⚠️ No Changeset found

Latest commit: fc5a6bd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Dec 26, 2024

✅ PR title follows Conventional Commits specification.

Copy link

codesandbox-ci bot commented Dec 26, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit fc5a6bd:

Sandbox Source
razorpay/blade: basic Configuration

@rzpcibot
Copy link
Collaborator

rzpcibot commented Dec 26, 2024

Bundle Size Report

No bundle size changes detected.

Generated by 🚫 dangerJS against fc5a6bd

- Less verbose compared to API without layout component

**Cons:**

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really a con? Lets check with design if this will ever be needed to move things around within quick filters

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup it might come in as we discussed earlier on call. Turning 2 individual inputs to InputGroup, moving items to other side, etc might come in future

## Enhancements / Components

- Enhancements:
- Searchable Dropdown
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isnt that just autocomplete? What do we need to do here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. It's AutoComplete inside DropdownOverlay instead of AutoComplete as trigger for Dropdown.

Something that we can use inside PhoneNumberInput as well to search for countries inside the Dropdown


- Enhancements:
- Searchable Dropdown
- InputGroup (TBD)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was needed for search but we decided to deprioritise it and do it later
image

- Enhancements:
- Searchable Dropdown
- InputGroup (TBD)
- New Components
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the number of new components and enhancements here, our estimate of 2 weeks seems less. Lets re-evaluate the estimates?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed it earlier in call. Also InputGroup is deprioritised and will be done later.

</Dropdown>
```

### FilterChipGroup
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be different types of filter chips, right? Will this API suffice for all the types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are different types of overlays but the trigger of FilterChip itself stays same.

In terms of overlays as well we decided that we'll just stick to DatePicker and Dropdown for now and publish menu later. Although the API itself is flexible enough to support Menu in future when needed

</Dropdown>
```

### FilterChipGroup
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the naming convention on Figma for this? Are we also call this FilterChipGroup & FilterChip?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: make names consistent on figma and dev

/* filter on search */
}}
>
<FilterGroup />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is FilterGroup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's FilterChipGroup component. Will be fixed in next commit

Comment on lines +396 to +398
onSearch={({ value, searchType }) => {
/* filter on search */
}}
Copy link
Member

@anuraghazra anuraghazra Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@saurabhdaware I believe we should keep dev side a bit more flexible with slots.

For example in many places in X we don't need search but rather we have some action buttons.

We can keep the QuickFilters and SearchGroup into separate slots which can be independently be used.

<ListView> // internally it will have justify-content: space-between
  <ListViewQuickFilters
    filters={[
      {
        title: 'All',
        onClick: () => {},
      },
      {
        title: 'Pending',
        onClick: () => {},
      },
    ]}
  />
  <Box>
    <ListViewFilterToggle />
    <ListViewSearchGroup />
  </Box>
</ListView>

not exactly but something like this ^ this way I can use QuickFilters independently of ListView.

Or

<ListView> // internally it will have justify-content: space-between
  <ListViewFilters>
    <QuickFilters
      filters={[
        {
          title: 'All',
          onClick: () => {},
        },
        {
          title: 'Pending',
          onClick: () => {},
        },
      ]}
    />
    <ListViewFiltersActions> // control the gap also
      <ListViewFilterToggle />
      <ListViewSearchGroup />
    </ListViewFiltersActions>
  </ListViewFilters>
</ListView>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed it that day. It'll make API a bit verbose and we still want to control the overall layout of this section on DS-side otherwise any change in position of items will require consumer migration. We can control it with some leading trailing type of component like we have in Card but they too will require migration if the terms Leading and Trailing become meaningless with new change.

```ts
type QuickFilter = {
title: string;
onClick: (e: React.MouseEvent) => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will also need onChange, value & controlled/uncontrolled state for QuickFilters.

Because QuickFilters will have the same semantics of RadioGroup it can also change on keyboard events not just click.

Plus this will also be a requirement if we want this to work independently as i've mentioned here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually checking if we need multiselect here - https://www.figma.com/design/ZZ2dpcIAsPCEGPwQ2UdgL1?node-id=84-132809&m=dev#1083598127

Otherwise we'll also need some type of quickFilterSelectionType prop as separate thing on ListViewFilters component 🤔

onClick: () => {},
},
]}
onSearchChange={() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure consumers are able to externally control all aspects of quickfilters/search etc.

Quick filters, search, and dropdowns may be controlled or have initial values set via URL query parameters.

Copy link
Member Author

@saurabhdaware saurabhdaware Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:

  • Check search feasibility (design @-RK and API)
  • Check what all might come in trailing of QuickFilters

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Search will be optional
  • Counter, Badge, etc

Comment on lines 280 to 298
<FilterChipGroup>
<Dropdown>
<FilterChip value={duration} onClearButtonClick={({ value }) => setDuration(undefined)}>
Duration
</FilterChip>
<DropdownOverlay>
<ActionList>
<ActionListItem
title="2 days"
value="2d"
onClick={({ name, value }) => setDuration(name)}
/>
<ActionListItem
title="1 week"
value="1w"
onClick={({ name, value }) => setDuration(name)}
/>
<ActionListItem
title="1 month"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn’t this get quite cumbersome to write all these JSX even when we just have 2-3 filter chips, not to mention most production apps will have 8-12 chips

Are there any alternatives we can approach? 🤔 Since we already know the specific inputs involved, could we consider a more streamlined, config-driven, and focused API instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check with RK on where we need freeflowing texts + leading / trailing usage on actionlist

Copy link
Member Author

@saurabhdaware saurabhdaware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore the irrelevant comments. Old comments. I forgot to submit these.

onClick: () => {},
},
]}
onSearchChange={() => {
Copy link
Member Author

@saurabhdaware saurabhdaware Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:

  • Check search feasibility (design @-RK and API)
  • Check what all might come in trailing of QuickFilters

```ts
type QuickFilter = {
title: string;
onClick: (e: React.MouseEvent) => void;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Will change

Comment on lines 280 to 298
<FilterChipGroup>
<Dropdown>
<FilterChip value={duration} onClearButtonClick={({ value }) => setDuration(undefined)}>
Duration
</FilterChip>
<DropdownOverlay>
<ActionList>
<ActionListItem
title="2 days"
value="2d"
onClick={({ name, value }) => setDuration(name)}
/>
<ActionListItem
title="1 week"
value="1w"
onClick={({ name, value }) => setDuration(name)}
/>
<ActionListItem
title="1 month"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check with RK on where we need freeflowing texts + leading / trailing usage on actionlist

</Dropdown>
```

### FilterChipGroup
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: make names consistent on figma and dev


#### Props

##### ListView
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weren't we going to make the search optional? Can't see any prop to hide search

- If we go with Layout component, then the tradeoff is the educational effort of teaching consumer which part of the UI comes directly from blade as layout component.
- If we go without Layout component, tradeoff is not being able to update the layout changes on consumer if something updates in dev.

- ### InputGroup
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this resolved now? We concluded to not do input group right?

- FilterView
- TableFilterView

- ### Do we want to control layout or not?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is resolved right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants